Fix/Add Patches 13 (2D modal) and 14 (3D spherical harmonics)#1163
Open
ChrisZYJ wants to merge 6 commits intoMFlowCode:masterfrom
Open
Fix/Add Patches 13 (2D modal) and 14 (3D spherical harmonics)#1163ChrisZYJ wants to merge 6 commits intoMFlowCode:masterfrom
ChrisZYJ wants to merge 6 commits intoMFlowCode:masterfrom
Conversation
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughThis PR expands immersed-boundary support by adding 2D modal (Fourier) and 3D spherical-harmonic patch geometries, with associated data, initialization, validation, MPI broadcasts, math helpers, docs, examples, and test/golden data. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Case as Case file / Example
participant Parser as Parameter parser
participant Validator as m_check_patches
participant MPI as m_mpi_proxy
participant Init as m_icpp_patches
participant Math as m_helper (real_ylm)
participant Grid as Grid population
Case->>Parser: Load `patch_icpp(i)` params (geometry, coeffs)
Parser->>Validator: Validate patch geometry & parameter ranges
Validator-->>Parser: Validation result (ok / error)
Parser->>MPI: Broadcast patch params (fourier_*, sph_har_coeff, modal flags)
MPI->>Init: Deliver parameters to all ranks
Init->>Math: Evaluate Y_lm(θ,φ,l,m) or modal Fourier terms per grid point
Math-->>Init: Return basis values
Init->>Grid: Compute R_boundary and populate patch state in mesh cells
Grid->>Grid: Finalize initialization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/1967AE06/golden-metadata.txt (1)
7-7: Golden metadata generated from a dirty working tree.The git state shows
(dirty), meaning there were uncommitted changes when this golden file was generated. Consider regenerating the golden files from a clean commit to ensure the test baselines are reproducible and traceable. Based on learnings: "Run./mfc.sh testlocally before requesting review" and "Use./mfc.sh test --generate -o <test_id>to create golden files for new test cases."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/1967AE06/golden-metadata.txt` at line 7, The golden metadata shows a "(dirty)" git state which means it was generated with uncommitted changes; to fix, reset or commit your working tree so it's clean, regenerate the golden file for the test ID 1967AE06 by running the referenced test-generation command (e.g. run ./mfc.sh test --generate -o 1967AE06 or run ./mfc.sh test locally) and replace tests/1967AE06/golden-metadata.txt with the new output so the Git line no longer contains "(dirty)" and the baseline is reproducible.examples/2D_patch_modal_shape_exp/case.py (1)
7-7: Nit: DefineLyexplicitly for clarity.
Lxis used for bothlength_xandlength_y(line 74). The additive-form example defines bothLx, Ly. For consistency and readability, consider definingLy = 8.0explicitly here too.Proposed fix
-Lx = 8.0 +Lx, Ly = 8.0, 8.0and
- "patch_icpp(1)%length_y": Lx, + "patch_icpp(1)%length_y": Ly,Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/2D_patch_modal_shape_exp/case.py` at line 7, Define Ly explicitly to match Lx instead of reusing Lx for both dimensions: add a line setting Ly = 8.0 and update any places that set length_y (or pass length_y) to use Ly (e.g., where length_x and length_y are supplied) so the mesh/patch geometry uses Lx and Ly separately; update the example to mirror the additive-form pattern.src/common/m_derived_types.fpp (1)
219-232: Verify all three executables after extendingic_patch_parameters.Since this type lives in
src/common, please confirm pre_process, simulation, and post_process builds/tests still pass with the new fields.Based on learnings: "Code in
src/common/affects all three executables (pre_process, simulation, post_process). Changes here have wide blast radius and require testing all three targets."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/m_derived_types.fpp` around lines 219 - 232, You extended the derived type used across executables (see ic_patch_parameters in m_derived_types.fpp and nearby fields like fourier_cos, fourier_sin, modal_clip_r_to_min, modal_r_min, modal_use_exp_form, sph_har_coeff), so rebuild and run all three targets: pre_process, simulation, and post_process; fix any compile/link errors by updating all modules and call sites that reference ic_patch_parameters (parsing, I/O/serialization, initial-condition setup), update any input/config files or unit tests that instantiate this type, and run the test suites for each executable to confirm no runtime failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/documentation/case.md`:
- Around line 1105-1117: The inline LaTeX math in the Geometry 13 documentation
uses $...$ which fails the linter; update every inline occurrence to use the
sphinx-friendly \f$...\f$ form (e.g., replace θ = atan2(...) written as $...$
with \f$...\f$ and similarly for R_boundary, radius, fourier_cos, fourier_sin,
modal_use_exp_form, modal_clip_r_to_min, modal_r_min expressions); do not change
the math content, only swap the inline delimiters throughout that paragraph so
the formulas remain identical but use \f$...\f$.
In `@src/pre_process/m_icpp_patches.fpp`:
- Around line 1042-1152: Both s_icpp_2d_modal and s_icpp_3d_spherical_harmonic
are missing the analytic/hardcoded override hooks used by other patch routines;
add the same call sequence used elsewhere so analytic expressions or hardcoded
patches can override before falling back to s_assign_patch_primitive_variables.
Specifically, at the site where you currently call
s_assign_patch_primitive_variables (inside the if that checks r<=R and
alter_patch/smooth_patch_id), first invoke the project's analytical override
hook and the hardcoded override hook (the same functions/calls used by other
s_icpp_* routines), and only call s_assign_patch_primitive_variables when those
hooks did not perform an override; make the change in both s_icpp_2d_modal and
s_icpp_3d_spherical_harmonic.
In `@tests/87FBC893/golden-metadata.txt`:
- Around line 5-7: The golden-metadata.txt files contain machine-specific
absolute paths and git state and should not be tracked; update .gitignore to
stop forcing these files into the repo by removing or changing the negated
pattern `!/tests/*/golden-metadata.txt` (or add an explicit ignore entry for
tests/*/golden-metadata.txt), then untrack existing committed files (e.g., git
rm --cached tests/*/golden-metadata.txt) and commit the change so future runs
don’t produce spurious diffs; ensure any CI that relied on these files is
adjusted to regenerate them as needed.
---
Duplicate comments:
In `@tests/3F864CFF/golden-metadata.txt`:
- Around line 5-7: The golden-metadata file contains machine-specific and
working-tree state data (lines starting with "Invocation:", "Lock:", and "Git:")
that makes tests non-deterministic; update the metadata generation so it does
not embed local invocation paths or the dirty branch state: remove or normalize
the "Invocation:" path, remove machine-specific "Lock:" details or replace with
stable feature flags, and ensure "Git:" records a stable commit hash only (no
branch name or "dirty" marker) or use a placeholder when the repo isn't clean.
Locate the code that writes golden-metadata.txt (search for writers emitting the
"Invocation:", "Lock:", or "Git:" strings) and change it to emit
normalized/placeholder values or omit these lines so the golden file is
machine-independent.
---
Nitpick comments:
In `@examples/2D_patch_modal_shape_exp/case.py`:
- Line 7: Define Ly explicitly to match Lx instead of reusing Lx for both
dimensions: add a line setting Ly = 8.0 and update any places that set length_y
(or pass length_y) to use Ly (e.g., where length_x and length_y are supplied) so
the mesh/patch geometry uses Lx and Ly separately; update the example to mirror
the additive-form pattern.
In `@src/common/m_derived_types.fpp`:
- Around line 219-232: You extended the derived type used across executables
(see ic_patch_parameters in m_derived_types.fpp and nearby fields like
fourier_cos, fourier_sin, modal_clip_r_to_min, modal_r_min, modal_use_exp_form,
sph_har_coeff), so rebuild and run all three targets: pre_process, simulation,
and post_process; fix any compile/link errors by updating all modules and call
sites that reference ic_patch_parameters (parsing, I/O/serialization,
initial-condition setup), update any input/config files or unit tests that
instantiate this type, and run the test suites for each executable to confirm no
runtime failures.
In `@tests/1967AE06/golden-metadata.txt`:
- Line 7: The golden metadata shows a "(dirty)" git state which means it was
generated with uncommitted changes; to fix, reset or commit your working tree so
it's clean, regenerate the golden file for the test ID 1967AE06 by running the
referenced test-generation command (e.g. run ./mfc.sh test --generate -o
1967AE06 or run ./mfc.sh test locally) and replace
tests/1967AE06/golden-metadata.txt with the new output so the Git line no longer
contains "(dirty)" and the baseline is reproducible.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/documentation/case.md`:
- Around line 1105-1117: The docs lack a Geometry 14 description for 3D
spherical-harmonic surfaces; add a block matching Geometry 13 that explains
coordinate conventions and coefficient semantics: describe that Geometry 14 (3D
spherical harmonic) defines the surface as r(θ,φ)=radius + sum_{l,m}
sph_har_coeff(l,m) * Y_l^m(θ,φ) (or an exponential variant if you support
modal_use_exp_form), state the polar and azimuthal conventions θ = acos(z/r) and
φ = atan2(y,x) relative to the centroid, clarify that the implementation uses a
real-Ylm basis and list the coefficient indexing (l = 0..L, m = -l..l) and
whether coefficients are absolute (length) or relative (dimensionless) and any
clipping behavior analogous to modal_clip_r_to_min/modal_r_min.
---
Duplicate comments:
In `@src/pre_process/m_icpp_patches.fpp`:
- Around line 1146-1156: The s_icpp_3d_spherical_harmonic subroutine is missing
the same hooks/bookkeeping/Fypp macros and local declarations present in other
3D patch routines; add the same variable declarations (analytical flags,
Hardcoded3D markers, bookkeeping arrays/indices and any Fypp-generated locals)
near the existing declarations around where s_icpp_3d_spherical_harmonic begins
(matching the pattern used in s_icpp_2d_modal and other 3D routines), insert the
analytical/@:Hardcoded3D/Fypp macro calls and bookkeeping statements immediately
after the call to s_assign_patch_primitive_variables(patch_id, i, j, k,
eta_local, q_prim_vf, patch_id_fp) to mirror other patches, and ensure any
allocated bookkeeping arrays are deallocated before the end of the subroutine to
match the resource handling in the other 3D patch implementations.
- Around line 1086-1095: In s_icpp_2d_modal, add the missing
analytical/hardcoded hooks, bookkeeping update, and Fypp variable/deallocation
macros: declare the
HardcodedDimensionsExtrusion()/Hardcoded2DVariables()/HardcodedDellacation()
variables near the existing declarations at the top of the subroutine, after the
declaration block around line ~1056; immediately after the call to
s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf,
patch_id_fp) insert calls to @:analytical() and an if-block for @:Hardcoded2D()
and update the patch_id_fp bookkeeping (set patch_id_fp(i,j,0)=patch_id where
appropriate); finally add the Fypp deallocation macro invocation
(HardcodedDellacation()) before the end of subroutine s_icpp_2d_modal so the
hardcoded 2D variables are cleaned up.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1163 +/- ##
==========================================
+ Coverage 44.05% 44.43% +0.37%
==========================================
Files 70 70
Lines 20498 20504 +6
Branches 1990 1996 +6
==========================================
+ Hits 9030 9110 +80
+ Misses 10329 10245 -84
- Partials 1139 1149 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@sbryngelson I think this PR is ready. Could you please take a look? Thank you. |
Member
|
i need to force it to run benchmark for now, since i somehow partially broke that and also we now have many amdflang workarounds for case optimization (used in benchmark) |
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
Contributor
|
CodeAnt AI Incremental review completed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Description
This PR fixes the 3D spherical harmonics and adds a dedicated 2D modal patch type:
Geometry 13 is 2D modal (Fourier), with explicit
fourier_cos/fourier_sin, optional additive vs. exponential form, and optional radius clipping.Geometry 14 is 3D spherical harmonics and uses real spherical harmonics with an explicit sph_har_coeff(l,m) surface definition. It replaces the old spherical-harmonic subroutine, which had buggy and undocumented behavior. There are no backward compatibility issues, as the original Geometry 14 was not used in any examples or tests. This PR adds clear examples and documentation.
Type of change
Testing
Checklist
See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)CodeAnt-AI Description
Add a 2D Fourier modal boundary (geometry 13) and a corrected 3D real spherical-harmonic boundary (geometry 14)
What Changed
Impact
✅ New 2D modal (Fourier) boundary for planar problems✅ Clearer, correct 3D spherical-harmonic surfaces✅ Fewer division-by-zero errors near patch centroids💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.